Skip to content

[2.1] Remove PHPSESSID#8394

Merged
Sesquipedalian merged 14 commits intoSimpleMachines:release-2.1from
sbulen:21_remove_phpsessid
Jul 31, 2025
Merged

[2.1] Remove PHPSESSID#8394
Sesquipedalian merged 14 commits intoSimpleMachines:release-2.1from
sbulen:21_remove_phpsessid

Conversation

@sbulen
Copy link
Copy Markdown
Contributor

@sbulen sbulen commented Jan 3, 2025

Fixes #8393

Partial for #8383 (addresses 2.1 only)

This PR removes PHPSESSID URL handling altogether from SMF. It also addresses the 8.4 SID & session parameter deprecation issues, that are related. The point of the 8.4 changes was to eventually remove PHP's PHPSESSID URL processing.

It goes a bit further and will not write sessions where cookies are not present/allowed. I believe this last part can significantly decrease MySQL CPU workload for websites that are getting crawled heavily (like mine).

Testing thus far is fine. Things work OK with & without cookies. No impact to logon/logoff. No impact to queryless URLs. And there are DEFINITELY far fewer session records, that I believe were all unused anyway.

I have a modlet with this code that I have installed in various test environments & tested. It's on my prod site as well.

Loss of functionality:
I believe the only functions that are no longer supported are:

  • Guest posts with cookies disallowed
  • You cannot view who likes posts with cookies disallowed
  • Search verification with cookies disallowed (search works OK without verification)
  • New user registrations with cookies disallowed (not really a loss, because they can't login with cookies disallowed)

sbulen added 2 commits January 3, 2025 11:57
Signed by Shawn Bulen, bulens@pacbell.net
Signed by Shawn Bulen, bulens@pacbell.net
@sbulen sbulen changed the title 21 remove phpsessid [2.1] Remove PHPSESSID Jan 4, 2025
@sbulen
Copy link
Copy Markdown
Contributor Author

sbulen commented Jan 4, 2025

The PHP check error, I think, is due to the fact that in some of the 2.1 source code the year is still 2024...

Signed by Shawn Bulen, bulens@pacbell.net
@sbulen sbulen mentioned this pull request Jan 7, 2025
Signed by Shawn Bulen, bulens@pacbell.net
Comment thread Sources/Session.php Outdated
Signed by Shawn Bulen, bulens@pacbell.net
@sbulen
Copy link
Copy Markdown
Contributor Author

sbulen commented Jan 13, 2025

I've been running this on my prod forum since about 1/2/25, and since then I have not seen crazy spikes in CPU or 'most online' stats.

VGF-cpu-2025-01-13

@sbulen
Copy link
Copy Markdown
Contributor Author

sbulen commented Feb 7, 2025

I still have this running on my production forum, and the results are still excellent. Still no significant spikes in CPU or most online.

Even more surprising... Interest in our board jumps significantly upon certain product launches. We had to do a massive board index restructure to fold in the new products, and legit user visits have been sustained pretty high since the product announcement.

In the past, either of these circumstances, either the board restructure or the spike in interest due to a product launch, would have caused significant CPU spikes for a few weeks. We are seeing MUCH improved behavior...

@jdarwood007 jdarwood007 added this to the 2.1.6 milestone Feb 8, 2025
…psessid

Signed-off-by: Shawn Bulen <bulens@pacbell.net>
Copy link
Copy Markdown
Member

@Sesquipedalian Sesquipedalian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to have the error popup that you had suggested in the 3.0 version of this PR, if you are willing to write that.

sbulen added 4 commits April 29, 2025 15:57
Signed-off-by: Shawn Bulen <bulens@pacbell.net>
Signed-off-by: Shawn Bulen <bulens@pacbell.net>
Signed-off-by: Shawn Bulen <bulens@pacbell.net>
Signed-off-by: Shawn Bulen <bulens@pacbell.net>
@sbulen
Copy link
Copy Markdown
Contributor Author

sbulen commented Apr 30, 2025

Notes on the recent commits:

With cookies disabled, & PHPSESSID removed from URLs:

  • Captchas did not work - the image was missing on the screens. This affected guest posts, guest searches, & new user registrations.
  • Even when captchas were not required, guest posts did not work & registrations did not work - they errored out on session errors.
  • Guest searches work fine if captcha is not required.

Note that if you went to the action under these circumstances, the screen was incomplete - it was missing the capcha image. Since it can look quite odd/confusing, partially painting the screen of a function they can't use, it is a little tidier to just not let them go there.

Note also that the login logic needed a little tweaking - checking the session before loading the theme made it difficult to provide a meaningful 'check your cookies' error upon login. A workaround is proposed here.

…psessid

Signed-off-by: Shawn Bulen <bulens@pacbell.net>
@Rupurudu
Copy link
Copy Markdown
Contributor

I want to add that, I'm also running without PHPSESSID on my live forum for about ~15 months.

Here are the changes I have:

 Sources/QueryString.php | 3 +++
 Sources/Session.php     | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/Sources/QueryString.php b/Sources/QueryString.php
index 70588db27..74c6e3317 100644
--- a/Sources/QueryString.php
+++ b/Sources/QueryString.php
@@ -652,6 +652,9 @@ function ob_sessrewrite($buffer)
 	if ($scripturl == '' || !defined('SID'))
 		return $buffer;
 
+	// Please don't add session IDs to urls.
+	return $buffer;
+
 	// Do nothing if the session is cookied, or they are a crawler - guests are caught by redirectexit().  This doesn't work below PHP 4.3.0, because it makes the output buffer bigger.
 	// @todo smflib
 	if (empty($_COOKIE) && SID != '' && !isBrowser('possibly_robot'))
diff --git a/Sources/Session.php b/Sources/Session.php
index 5237af0b5..d84dcb083 100644
--- a/Sources/Session.php
+++ b/Sources/Session.php
@@ -29,7 +29,7 @@ function loadSession()
 
 	// Attempt to change a few PHP settings.
 	@ini_set('session.use_cookies', true);
-	@ini_set('session.use_only_cookies', false);
+	@ini_set('session.use_only_cookies', true);
 	@ini_set('url_rewriter.tags', '');
 	@ini_set('session.use_trans_sid', false);
 	@ini_set('arg_separator.output', '&amp;');

@Rupurudu
Copy link
Copy Markdown
Contributor

Also I want to add that, users without cookies are so rare in fact that I've yet to heard a single feedback from someone about the forum is broken.

As for the reason why I made this change, because I removed the index.php from my $scripturl, and this breaks the very first load on the page. I looked what ob_sessrewrite function did, and decided it's such an edge case that it's probably not needed for regular users.

Comment thread Sources/Session.php
@sbulen
Copy link
Copy Markdown
Contributor Author

sbulen commented May 30, 2025

Also I want to add that, users without cookies are so rare in fact that I've yet to heard a single feedback from someone about the forum is broken

Yep. Same here.

@Doyeip
Copy link
Copy Markdown

Doyeip commented Jun 5, 2025

I suggested this PR to our staff who implemented the changes in response to recent crawler/bot wave but ended up reverting. While there was a minor reduction in processor spikes with the patch, the staff received complaints that the forum had broken from various privacy minded members. We're looking at some kind of middle ground where cookie-free users can still use the forum but cookie-less crawlers don't end up mass generating sessions. We're reworking it to only generate a session if the user agent performs an action which requires a session (login, registration, guest captcha). That way bots loading up seemingly random pages on the forum from a crawl list don't generate sessions.

@jdarwood007
Copy link
Copy Markdown
Member

If you have analytic tools, check to see if your rise in traffic is AI-related. AI bots do not respect robots.txt or other normal bot-related optimizations, and the only way to counter them is with firewalls.

@sbulen
Copy link
Copy Markdown
Contributor Author

sbulen commented Jun 5, 2025

I suggested this PR to our staff who implemented the changes in response to recent crawler/bot wave but ended up reverting. While there was a minor reduction in processor spikes with the patch, the staff received complaints that the forum had broken from various privacy minded members. We're looking at some kind of middle ground where cookie-free users can still use the forum but cookie-less crawlers don't end up mass generating sessions. We're reworking it to only generate a session if the user agent performs an action which requires a session (login, registration, guest captcha). That way bots loading up seemingly random pages on the forum from a crawl list don't generate sessions.

Thanks for the feedback, helpful.

When folks said the forum seemed broken, what functions did not work, exactly? I believe the only functions that are no longer supported are:

  • Guests posts with cookies disallowed
  • Search verifications with cookies disallowed (search works OK without verification)
  • New user registrations with cookies disallowed (not really a loss, because they can't login with cookies disallowed)

Folks with cookies disabled could never login; this PR doesn't change that.

A few other things to keep in mind:

  • You cannot distinguish bots from users via useragent. Most of the problematic bots I see today use useragents that mimic users on browsers.
  • GET/POST sessions are deprecated in php8.4 & are gone by php9.0. I.e., this is coming one way or another. This PR prepares SMF for this eventuality.
  • The reason for deprecating url sessions is it is no longer considered safe/good practice. So... without a cookie... how are you going to lookup the session?

@sbulen
Copy link
Copy Markdown
Contributor Author

sbulen commented Jun 12, 2025

Another before & after, still looking good. Before, the forum spent 40%+ of its days at or above 40 mins CPU. Monthly, it would exceed 100+ mins for days on end. With this PR, it has reached 40 mins only twice in the last 3 months, and has not exceeded 41 mins in that timeframe.
VGF-cpu-2025-01-10
vgf_cpu_2025-06-12

@Sesquipedalian Sesquipedalian removed this from the 2.1.6 milestone Jun 25, 2025
@Sesquipedalian Sesquipedalian added this to the 2.1.7 milestone Jun 25, 2025
Signed-off-by: Shawn Bulen <bulens@pacbell.net>
@sbulen
Copy link
Copy Markdown
Contributor Author

sbulen commented Jul 6, 2025

I don't know why this still says 'changes requested', I think I addressed all of them.

@sbulen
Copy link
Copy Markdown
Contributor Author

sbulen commented Jul 11, 2025

I found one more limitation: a guest with cookies disallowed cannot view who liked a post. That passes a session via URL as well.

I'll tweak this over the next few days when I have time.

Signed-off-by: Shawn Bulen <bulens@pacbell.net>
@sbulen
Copy link
Copy Markdown
Contributor Author

sbulen commented Jul 14, 2025

The above issue was addressed. I kept the text, e.g., "2 people like this", but it is no longer a link.

Comment thread Themes/default/Display.template.php Outdated
Signed-off-by: Shawn Bulen <bulens@pacbell.net>
@jdarwood007
Copy link
Copy Markdown
Member

@sbulen Is this PR mergable yet or are you still expecting changes?

@sbulen
Copy link
Copy Markdown
Contributor Author

sbulen commented Jul 30, 2025

@jdarwood007 - I am OK with merging; I've had this on my site all year.

The comment above from @Doyeip gives me a bit of concern though. There may be ramifications to serious privacy-minded users. OTOH, from a PHP perspective, this is coming no matter what. And passing session by URL is clearly not a good thing to do... Still, folks are folks, & we may see a bit of pushback from those who disable cookies, e.g., TOR-browser-adjacent folks.

@jdarwood007
Copy link
Copy Markdown
Member

I would almost propose a service worker to retain the "session" token, but bots use real rendering engines and JavaScript processing just like a normal browser, and would continue to be able to pass on. But it would mean they would have to register the service worker to continue a normal browsing session, which seems like a step up from what I would expect more harmful bots to do.

Copy link
Copy Markdown
Member

@jdarwood007 jdarwood007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks fine. Shawn has done enough testing for me to call it safe.

@Sesquipedalian Sesquipedalian dismissed their stale review July 31, 2025 16:14

Requested changes made

@Sesquipedalian Sesquipedalian merged commit 28f6eef into SimpleMachines:release-2.1 Jul 31, 2025
10 checks passed
@sbulen sbulen deleted the 21_remove_phpsessid branch August 9, 2025 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[2.1] & [3.0]: Consider disabling URL-based sessions, getting rid of PHPSESSID from URLs

6 participants